fix(table): dispatch drag gestures that leave the widget, and measure travel against the content - #377
Merged
Merged
Conversation
Pointer travel was compared between two viewport points, so a gesture the content moved under — a scroll mid-drag, a host-driven scrollIntoView — measured near zero and was judged a plain click, dropping the selection. Take both endpoints relative to contentDOM's live rect instead. One frame, so a pointer that follows the moving content still measures zero and stays a click, and both operands are visual pixels — which matters because CodeMirror supports being CSS transformed, where mixing a client coordinate with a layout-space scroll offset would break the threshold.
…e widget A drag that starts in a rendered cell and is released outside the widget never delivered a click to the widget root — per UI-events the click retargets to the nearest common ancestor of the press and release targets, measured here as .cm-content — so the gesture was lost and the editor was left with whatever caret its observer happened to park at the release point. Add a second dispatch seam: a document-level mouseup, armed at mousedown, that dispatches only when the release landed outside the root. An inside release still belongs to the click listener, so the modifier-click open-external path is untouched. Four things disarm the seam, each pinned by a test that reddens when its guard is removed: the release itself, a native drag-and-drop (which ends in dragend, never mouseup), WidgetType.destroy, and the next press anywhere. That last one is registered in the CAPTURE phase deliberately — four sibling widgets stop mousedown propagation while leaving mouseup alone, and a bubble-phase disarm starved by one of those would let a stale gesture dispatch a range the user never drew.
The happy-dom suites drive the widget with hand-built MouseEvents, so none of them observes what a real pointer gesture delivers. These two rows use Playwright's own mousedown/mousemove/mouseup/click over real geometry. The emptiness assertion comes first deliberately: before the fix the editor already parked a collapsed caret at the release position, so asserting the head alone would have passed without the seam existing. The second row covers the overshoot below the last line, which the unit suite cannot see — posAtCoords clamps a point past the document to doc.length rather than answering null, so that gesture is a real range.
Type soundness: isDrag's false arm narrowed on conditions that are not
properties of its argument (detail and travel), so TS's negative inference
lied. Split into a pure isArmed guard plus armedDragFor, which returns a
value and therefore has no false arm at all.
Coordinate and offset spaces: contentPoint returned a bare {x, y},
structurally identical to the viewport points passed to posAtCoords and
cellPointAt two lines away. Named ContentPoint makes the mis-pass a compile
error. releaseRange was the one absolute-space entry in this family with no
asAbsoluteOffset mint, which the module's own policy requires be said out
loud; DragSelection now names the shape both seams produce.
Diagnostics: the release-lookup catch logged {err} alone, below the standard
this file sets twice. It now carries the cell offset and the coordinates.
Phase: the dispatching mouseup was bubble while both disarms were capture,
resting on an enumeration of today's siblings — the reasoning the adjacent
comment rejects. It is capture now. CodeMirror registers no document mouseup
for a gesture that starts in an ignoreEvent widget, so the ordering change is
inert; the browser suite measures unchanged at 64.
Coverage: four paths had no test — the forward arm of the outward snap
(a mutation to a constant left every row green), the release-lookup catch,
the multi-widget disarm ordering, and the fixture's posAtCoords misuse
channel. A byte-identical duplicate row that could not pin what it named is
replaced by one pinning re-arm after a completed gesture.
Comments: the file header documented only the click seam while the code had
two; 'sole dispatch seam' sat 35 lines above 'The SECOND dispatch seam'; and
several internal line references had rotted. Bare :NNN refs are replaced by
file-and-symbol references, which do not rot.
The two disarm listeners each spelled out the same abort-and-clear pair, which must stay identical — an abort without the pendingDrag clear leaves an anchor to be paired with someone else's release. One named local makes that structural rather than a convention, and records why the mouseup seam deliberately does not use it: an inside release must leave pendingDrag armed for the click listener that owns it. travelSince measures a distance, so it now takes the pressed point rather than the whole PendingDrag; the anchor's cell mapping is no business of it.
…t they missed The second review round found that the fixes from the first had reproduced the defect they were fixing: prose asserting things the code does not do. Two test comments claimed pins that measurement disproves. The two-table row said it pinned the WeakMaps' root-keying; making both maps module-wide singletons leaves the whole suite green, because the capture-phase disarm runs before the second root's bubble arm, so even a shared map sees clear-then-write-then-read. It pins the coexistence of disarm and arm, and says so now — with two new rows that do pin the keying, verified red under that mutation. The re-arm row claimed no single-line mutation could redden it. Two can. One works here; the other should work and does not, because happy-dom registers listeners on a pre-aborted AbortSignal where the DOM spec requires a no-op — which is why the earlier claim was believed. Both are recorded, with the general rule that a green mutation is not evidence of a missing pin in this environment until the event fidelity is ruled out. The release-lookup payload widened in cycle 1 was asserted so loosely that all three new fields could be deleted with the suite green; it now pins the values, which also catches logging the wrong cell. Also: a mint comment claimed to be the family's only unbranded entry while the caret path beside it is unbranded too (unchanged by this PR, and now tracked separately); asAbsoluteOffset's docblock enumerated its call sites and the enumeration had already gone stale; isArmed read as 'a gesture is armed' when it means 'the press landed on a cell'; and the ordinal, unit and scope of three further comments were wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two edge cases in table-widget drag selection, both of which fell back to a caret and silently lost the gesture.
A drag released outside the widget dispatched nothing. Per UI Events a
clickis delivered to the nearest common ancestor of the press and release targets, so a drag that starts in a rendered cell and ends outside the widget never reaches the root's click listener — which was the only dispatch seam. Measured in real Chromium:mouseuplands on.cm-line,clickon.cm-content. The user-visible symptom was not "nothing happens" but "a caret appeared at the release point", because CodeMirror's own observer parks one there.The drag/click discrimination measured the wrong reference frame. Travel was compared between two
clientX/Ypairs, so a gesture the content moved under (a scroll mid-drag, a host-drivenscrollIntoView) measured near zero and was judged a plain click.What changed
mouseup, armed at mousedown, that dispatches only when the release lands outside the root. An inside release still belongs toclick, so the modifier-clickopen-externalpath is untouched — pinned by a test that drives the whole press/release/click gesture on an in-cell link.view.contentDOM's live rect. One frame, so a pointer that follows the moving content still measures zero and stays a click, and both operands are visual pixels — which matters because CodeMirror supports being CSS transformed (view.scaleXexists for that), where mixing a client coordinate with a layout-space scroll offset breaks the threshold.AbortController: the release itself,dragstart(a native drag-and-drop ends indragend, nevermouseup),WidgetType.destroy, and the next press anywhere.The part worth reviewing closely
The disarm listener is registered in the capture phase, and that is load-bearing rather than stylistic. Four sibling widgets in this editor call
stopPropagation()onmousedown— the task checkbox, the fenced-code copy and collapse buttons, the language picker — and none of them stopsmouseup. A bubble-phase disarm is starved by exactly those presses while the release still arrives, which is the one combination that dispatches a range the user never drew:mousedownis stopped, so a bubble disarm never runs;mouseupis not stopped → the stale listener fires → range from the old cell to the checkbox.Measured against a
stopPropagation-ing element: bubble fired 0, capture 1, and themouseuparrived regardless. The phase split is principled — starving the seam costs a gesture (and the next press disarms it anyway), while starving the disarm invents a selection.Tests
cm-table-widget-release.test.ts(16 rows): the outside release, the inside release deferring toclick, the modifier-link gesture end to end, and every degrade and disarm path.posAtCoordsclamps todoc.lengthrather than answering null, so that gesture is a real range).Non-vacuity was verified by deleting each guard in turn and confirming the matching row reddens, rather than assumed. The disarm rows pass vacuously until the seam exists, so their red-first evidence is on the deletion side; that is stated in the test file rather than left implicit. One guard — the arming-press identity check — provably cannot be pinned under the capture phase and is labelled as defence in depth instead of being given a test that cannot fail.
Verification
pnpm compile·pnpm lint· 5063 unit · 64 browser · 103 e2e ·pnpm build·pnpm package(vsix audit clean) · force-installed.